-
Notifications
You must be signed in to change notification settings - Fork 691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand and unify --keep-temp-files
#10292
base: master
Are you sure you want to change the base?
Expand and unify --keep-temp-files
#10292
Conversation
69f0104
to
1e05052
Compare
One concern I have is that I don't fully understand how the I'm not objecting to this PR (which I am happy to see), but I would like to make sure that you have taken into consideration the logical separation between the Cabal library and |
Yes, I wasn't entirely sure where to put this flag. There seem to be a lot of flags that are shared between multiple commands (like I picked |
e0ea1a0
to
4156c93
Compare
{ -- Cabal < 3.13 does not support the --working-dir flag. | ||
setupWorkingDir = NoFlag | ||
, -- Or the --keep-temp-files flag. | ||
setupKeepTempFiles = NoFlag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is accurate, some commands did support --keep-tmp-files
before?
What happens if you are using repl -keep-tmp-files
with an older Cabal version? Is that broken by this patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some commands did, in an ad hoc fashion; my understanding is that the point of this patch is to extend that to all commands and ensure they behave consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also #10209.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test that I believe will exercise this case and run it with --boot-cabal-lib
:
Let me know if this is sufficient, or if I missed a detail.
Here's the patch to validate.sh
I used:
diff --git a/validate.sh b/validate.sh
index b22e033f8..40c4bfcb1 100755
--- a/validate.sh
+++ b/validate.sh
@@ -505,7 +505,7 @@ CMD="$($CABALLISTBIN cabal-install:test:integration-tests2) -j1 --hide-successes
step_cli_suite() {
print_header "cabal-install: cabal-testsuite"
-CMD="$($CABALLISTBIN cabal-testsuite:exe:cabal-tests) --builddir=$CABAL_TESTSUITE_BDIR --with-cabal=$($CABALLISTBIN cabal-install:exe:cabal) $TESTSUITEJOBS --with-ghc=$HC --hide-successes --intree-cabal-lib=$PWD --test-tmp=$PWD/testdb $RTSOPTS"
+CMD="$($CABALLISTBIN cabal-testsuite:exe:cabal-tests) --builddir=$CABAL_TESTSUITE_BDIR --with-cabal=$($CABALLISTBIN cabal-install:exe:cabal) $TESTSUITEJOBS --with-ghc=$HC --hide-successes --boot-cabal-lib $RTSOPTS PackageTests/MultiRepl/CustomSetupKeepTempFiles/cabal.test.hs"
(cd cabal-testsuite && timed $CMD) || exit 1
}
I think the patch looks good, apart from I think it won't work if you are using a custom setup built with an older version of Cabal. See my comment. |
We need an old-ghcs test job with custom setup, as we already have backward compatibility breakage that leaked through (#10379). |
4156c93
to
8255fc1
Compare
This lets the `--keep-temp-files` options to `cabal haddock` and `cabal repl` work with an older Cabal version. Pointed out by @mpickering: haskell#10292 (comment)
b209b2f
to
d797bc4
Compare
This lets the `--keep-temp-files` options to `cabal haddock` and `cabal repl` work with an older Cabal version. Pointed out by @mpickering: haskell#10292 (comment)
d797bc4
to
8cefa2c
Compare
This lets the `--keep-temp-files` options to `cabal haddock` and `cabal repl` work with an older Cabal version. Pointed out by @mpickering: haskell#10292 (comment)
8cefa2c
to
e9016e8
Compare
In haskell#10292, we will move the `--keep-temp-files` setting into `CommonSetupFlags` and out of `ReplFlags`/`HaddockFlags`. This means that the flag-filtering behavior (which adapts flags from new versions of `cabal-install` to old version of `Cabal`) will need to know which command is being run to provide the correct `CommonSetupFlags`. Therefore, this change adds several new `filterFooFlags` functions to provide this behavior, and removes the `commonFlags` used for all subcommands in `Distribution.Client.ProjectBuilding.UnpackedPackage`.
e9016e8
to
f7369cf
Compare
This PR has gotten too large to effectively review, so I've split out many of the changes into separate, smaller PRs:
Once those PRs are all merged, this change will only have ~100 lines to review. |
f7369cf
to
3f8a3d3
Compare
`addFields` should be in `ParseUtils` with the rest of the field helpers.
3f8a3d3
to
de33b8f
Compare
Currently, `cabal repl` has a `--keep-temp-files` option, and `cabal.project` has a `keep-temp-files` option but it only effects Haddock builds. This patch adds `--keep-temp-files` to `CommonSetupFlags`, making it available to all commands. The expanded `--keep-temp-files` flag is used for the `cabal repl` command and Haddock builds (retaining compatibility with the previous behavior) but is also used to determine when to keep response files.
de33b8f
to
facc238
Compare
This is blocking #9367.
Currently,
cabal repl
has a--keep-temp-files
option, andcabal.project
has akeep-temp-files
option but it only affects Haddock builds.This patch adds
--keep-temp-files
toCommonSetupFlags
, making it available to all commands. The expanded--keep-temp-files
flag is used for thecabal repl
command and Haddock builds (retaining compatibility with the previous behavior). The flag will also be used for response files; see #9367.Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
Depends-on: #10395
Depends-on: #10392
Depends-on: #10391
Depends-on: #10390
Depends-on: #10389
Depends-on: #10394
Depends-on: #10388
Depends-on: #10393